fix(users): reset email verification on email change (#3825)#3854
Conversation
Changing the account email via the self or admin update paths kept
emailVerified:true on the brand-new address, letting the OAuth
link-by-email flow ({ email, emailVerified: true }) attach a provider
identity to an address the account never proved it owns. On email
change (mailer configured), reset emailVerified, mint a fresh
verification token (24h expiry, same expressions as signup) and
fire-and-forget a verify-email mail to the new address. The recover
path (internal verification writer) and mailer-less deployments
(signup auto-verifies by design) are exempt. Re-sends are covered by
the existing resend-verification endpoint.
refs #3825
|
Warning Review limit reached
More reviews will be available in 15 minutes and 35 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3854 +/- ##
==========================================
+ Coverage 91.96% 91.98% +0.02%
==========================================
Files 160 160
Lines 5289 5303 +14
Branches 1698 1702 +4
==========================================
+ Hits 4864 4878 +14
Misses 337 337
Partials 88 88
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
users.service.js now imports logger (+ mailer + getBaseUrl) for the email-change reset path. The existing count unit test uses a minimal config mock (no `log` key); logger.js calls setupFileLogger() at module-evaluation time, which crashed on config.log.fileLogger. Add jest.unstable_mockModule stubs for the three new deps so the minimal-config test keeps running in isolation.
There was a problem hiding this comment.
Pull request overview
This PR addresses #3825 by invalidating a user’s verified-email state when their email address changes, preventing OAuth linkProviderByEmail from linking identities to an email the account no longer controls.
Changes:
- Update
UserService.update()to detect email changes, resetemailVerified, mint a new verification token/expiry, and (best-effort) send a re-verification email. - Add a new integration test suite covering self-update and admin-update email-change behavior and the OAuth linking guard.
- Adjust an existing unit test to mock newly introduced dependencies (logger/base URL/mailer) during module import.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
modules/users/services/users.service.js |
Adds email-change detection + verification reset/token minting + best-effort verification email send. |
modules/users/tests/users.email.change.integration.tests.js |
New integration coverage for email-change behavior across mailer-on/off and admin/self paths. |
modules/users/tests/users.service.count.unit.tests.js |
Adds mocks to keep unit tests stable after new service-level imports. |
Pin that the recover update path never resets emailVerified or sends a re-verification mail (verifyEmail invariant); complete the count unit-test mailer mock with isConfigured. refs #3825
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Summary
PUT /api/users(self-update) and the admin user-update path now resetemailVerifiedtofalse, mint a fresh email-verification token, and re-send the verification email when the email address changes — and the mailer is configured. Therecoverpath (the internal verification writer) is exempt.linkProviderByEmailtrusts{ email, emailVerified: true }. On a mailer-configured deploymentemailVerifiedis a real ownership proof, so an attacker could change their verified address to a victim's, complete a new OAuth handshake, and be linked/annexed. ResettingemailVerifiedon email change (then re-verifying) closes that surface.emailVerifiedis not an ownership proof there. Resetting it on email change would (a) permanently un-verify the user with no re-verify path (no mailer), breaking OAuth linking, and (b) not close any surface anyway — an attacker would auto-verify the victim address directly at signup, never via a change. So the reset is correctly gated onmailer.isConfigured().Scope
modules/users(service + integration/unit tests)nonelow— additive guard; fires only on an actual email-address change on a mailer-configured deploymentValidation
npm run lintusers.email.changeintegration suite green; full integration + e2e greenTest plan
New suite:
modules/users/tests/users.email.change.integration.tests.jsemailVerified→false; freshemailVerificationTokenminted; verify-email re-sentemailVerifiedunchanged, no token, no mailemailVerifiedunchanged (idempotent)emailVerifiedstaystrue— intentional exemption (see Why)emailVerifiedreturns totruerecoverupdateemailVerifiedor send mail (verifyEmail invariant).catch)Guardrails check
Notes for reviewers
/critical-reviewreturned OK-with-nits (0 critical/high). Copilot threads resolved.optionelse-armlogger.warn.